Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump dependencies on Delphi packages #12

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

brookslogan
Copy link
Contributor

@brookslogan brookslogan commented Dec 15, 2023

Summary: bump epidatr & epipredict dependency versions, fix breaking changes, update _freeze.

Breaking changes:

  • Some obvious renames.
  • Replace both pivot_quantiles and unnest pivot_wider patterns to
    pivot_quantiles_wider.
  • Fix some epi_recipe and frosting printing that doesn't play well with
    knitr now. (Workaround from here.)

_freeze updates seemed like they might have both trivial and nontrivial changes. I've tried to divide between commits based on whether I thought were significant vs. those that potentially could be pruned if they seem trivial on a second pass.

Some other notes / points to check; if someone else can take a second look that would be great:

  • Forecast generation dates printed in flatline forecaster chapter changed. I didn't notice other things in this chapter changing, but it might be worth a check to make sure something crazy isn't being plotted accidentally. (More than just the timestamp printing format, which is how some/all of the similar parts in other chapters changed.)
  • The tidymodels regression chapter's model fit and plots seem to have changed!
  • lm_wf_log %>% fit(penguins) output is missing step numbering still (messages probably being filtered out), but it's inside a larger chunk and I didn't want to duplicate it to apply the workaround; it looks fine without the numbering anyway.
  • Tons of references may have vanished from the rendered site, though that's not reflected in the .bib diff.
  • There were also some additional .bib changes that removed version numbers and potentially altered URLs for some of the packages; I'm not sure if that's due to my R version being behind the one used to generate this before, so I haven't included those changes here.

Bump versions, update renv.lock.

Adjust for breaking changes:
- Some obvious renames.
- Replace both `pivot_quantiles` and `unnest` `pivot_wider` patterns to
  `pivot_quantiles_wider`.
- Fix some `epi_recipe` and `frosting` printing that doesn't play well with
  knitr now.

Update _freeze files where a first pass visual diff identified
"real" differences rather than just a tiny visual offset.
A first pass visual diff did not identify any "real" differences with the
current versions, and suggests that some of these changes might just be a tiny
offset and/or size adjustment, but this would need a double check before trying
to avoid the updates to the _freeze & corresponding repo bloat.
dshemetov and others added 3 commits May 1, 2024 15:01
* remove references to R6 and mutation
* use epiprocess correctly
* fix the authors section of DESCRIPTION
* upgrade renv
* update all packages in renv
* integrate Rprofile with user Rprofile
feat: update for epiprocess R6 refactor
* add a README file
* fix broken formatting in packages.bib
* get missing data for sliding-forecasters.qmd online
  instead of local files
@dshemetov
Copy link

@dajmcdon this should be good to go. Mostly just updating the code to the most up to date versions of our packages. This includes my updates to the new epiprocess sans R6. Shouldn't need much review.

_common.R Outdated Show resolved Hide resolved
archive.qmd Outdated Show resolved Hide resolved
epiprocess.qmd Outdated Show resolved Hide resolved
#| echo: false
#| collapse: true
extract_recipe(out_gb$epi_workflow)
with_messages_cat_to_stdout(print(extract_recipe(out_gb$epi_workflow)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic comment:

with all these patterns, we lose the display of extract_recipe() (or similar) usage. Maybe we want to think about showing the feature in 1 chunk and printing with a second chunk?

Copy link
Contributor Author

@brookslogan brookslogan Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, you want both code saying

extract_recipe(........)

and the better-formatted output from with_messages_......... that doesn't turn it into a bunch of chunks?

I think current state of PR should do that. Every replacement was of something that was already echo: false, except for five_days_ahead$epi_workflow, which was split into a results: false chunk + echo: false chunk.

[That is, unless the pre-existing material was missing some results: false chunks where we actually want them. I think I just assumed they were omitted to avoid clogging the page.]

}

@Manual{R-epipredict,
title = {epipredict: Basic epidemiology forecasting methods},
author = {Daniel McDonald and Ryan Tibshirani and Logan Brooks and Rachel Lobay and Maggie Liu and Ken Mawer and Chloe You},
author = {Daniel McDonald and Ryan Tibshirani and Logan Brooks and Rachel Lobay},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to rethink author credit in both of our packages. These both strike me as overly exclusive.

dshemetov and others added 2 commits May 6, 2024 14:32
Co-authored-by: Daniel McDonald <[email protected]>
Co-authored-by: Daniel McDonald <[email protected]>
@dshemetov dshemetov mentioned this pull request Aug 20, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants